Conversation
…er configuration This configuration used to be hardcoded in crossgen2, but it workaround was recently reverted with proper support added in dotnet/runtime@5b11923. This means r2r_interpreter configuration needs to pass `--target-allows-runtime-code-generation:false` to crossgen. This can be achieved by having the bdn autogenerated project receive the msbuild property `PublishReadyToRunCrossgen2ExtraArgs` initialized to this extra arg. Given bdn doesn't support this configuration via its cli arguments (https://github.com/dotnet/BenchmarkDotNet/blob/master/src/BenchmarkDotNet/ConsoleArguments/CommandLineOptions.cs), we add a new argument to the microbenchmark project --msbuild-argument, which will be forwarded to bdn via the job arguments (job.WithMsBuildArguments).
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the performance microbenchmark harness so the coreclr_r2r_interpreter configuration can disable runtime code generation by passing an extra MSBuild property through BenchmarkDotNet to Crossgen2 (PublishReadyToRunCrossgen2ExtraArgs=--target-allows-runtime-code-generation:false).
Changes:
- Add an optional
msBuildArgumentsinput toRecommendedConfig.Createand apply it to the BDNJobviaWithMsBuildArguments. - Extend
src/benchmarks/microargument parsing to accept a new--msbuild-argumentsflag and forward the values intoRecommendedConfig.Create. - Update
scripts/run_performance_job.pyto supply the required MSBuild property whenruntime_type == coreclr_r2r_interpreter.
File summaries
| File | Description |
|---|---|
| src/harness/BenchmarkDotNet.Extensions/RecommendedConfig.cs | Accepts optional MSBuild arguments and applies them to the BenchmarkDotNet job configuration. |
| src/benchmarks/micro/Program.cs | Adds parsing/forwarding of a new CLI flag to inject MSBuild arguments into the benchmark job. |
| scripts/run_performance_job.py | Supplies the R2R interpreter Crossgen2 extra arg via the new MSBuild-arguments pathway. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@DrewScoggins Note that passing this crossgen2 argument to an older version that doesn't support it will simply crash the r2r execution. Not sure how this versioning is handled, whether we are guaranteed that dotnet/performance main is used only with latest main from dotnet/runtime and not with older versions. |
|
We do use dotnet/performance main for running all of our dotnet/runtime runs, so compatibility with release branches is important, but we are not doing r2r interpreter runs on release branches. https://github.com/dotnet/performance/blob/main/eng/pipelines/runtime-perf-jobs.yml#L370-L386. Maybe we should turn these runs on for release/11.0 branch but I think this is fine for now. If we start needing to take these into account, we can add a check or something then. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Preserve existing job arguments and add regression coverage for the exact MSBuild argument parsing path.
Review details
Suppressed comments (2)
src/benchmarks/micro/Program.cs:35
- The new forwarding path depends on the non-obvious delimiter behavior in
ParseAndRemoveStringsParameter: the MSBuild token must begin with/even though its value contains an embedded--. The existing tests only cover ordinary filter values, so a regression here could leave this argument in BDN and prevent the R2R configuration from being applied; add a test using the exact/p:PublishReadyToRunCrossgen2ExtraArgs=--target-allows-runtime-code-generation:falsevalue and verify it is returned and removed.
argsList = CommandLineOptions.ParseAndRemoveStringsParameter(argsList, "--msbuild-arguments", out msBuildArguments);
src/harness/BenchmarkDotNet.Extensions/RecommendedConfig.cs:49
WithMsBuildArgumentsreplaces the job's entireInfrastructure.Argumentscollection rather than appending to it. If a caller combines the new parameter with a custom job that already hasWithArguments(...)values, those existing MSBuild/runtime arguments are silently dropped. Preserve the existing arguments when adding these entries (or explicitly reject that combination).
if (msBuildArguments is not null && msBuildArguments.Count > 0)
{
job = job.WithMsBuildArguments(msBuildArguments.ToArray());
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
This configuration used to be hardcoded in crossgen2, but it workaround was recently reverted with proper support added in dotnet/runtime@5b11923. This means r2r_interpreter configuration needs to pass
--target-allows-runtime-code-generation:falseto crossgen. This can be achieved by having the bdn autogenerated project receive the msbuild propertyPublishReadyToRunCrossgen2ExtraArgsinitialized to this extra arg.Given bdn doesn't support this configuration via its cli arguments (https://github.com/dotnet/BenchmarkDotNet/blob/master/src/BenchmarkDotNet/ConsoleArguments/CommandLineOptions.cs), we add a new argument to the microbenchmark project
--msbuild-arguments, which will be forwarded to bdn via the job arguments (job.WithMsBuildArguments).